Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: migrate block definitions to Config API #42

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

Swanand01
Copy link
Contributor

@Swanand01 Swanand01 commented Feb 5, 2025

What

This PR moves the custom block definitions to the snapwp.config.ts file.
Instead of passing them as props to the EditorBlockRenderer, they are now imported from getConfig

Why

Since block definitions are overloadable/configurable, they should be put in the snapwp.config.ts file.

Related Issue(s):

https://github.com/orgs/rtCamp/projects/141/views/14?pane=issue&itemId=96524753&issue=rtCamp%7Cheadless%7C396

How

The blockDefinitions property is added to the SnapWPConfig interface and SnapWPConfigManager.snapWPConfigSchema

Testing Instructions

  1. Create a file named core-quote.tsx in examples/nextjs/starter/src/app.
  2. Override the block implementation in this file
  3. In snapwp.config.ts, add:
import type { SnapWPConfig } from '@snapwp/core/config';
import CoreQuote from './src/app/core-quote';
import { BlockDefinitions } from '@snapwp/types';

const blockDefinitions: BlockDefinitions = {
	CoreQuote: CoreQuote,
};

const config: SnapWPConfig = {
	blockDefinitions,
};

export default config;
  1. Add the Quote block to a page, and open the page from the next app.
  2. You should see the overridden implementation.

Screenshots

Additional Info

Checklist

  • I have read the Contribution Guidelines.
  • My code is tested to the best of my abilities.
  • My code passes all lints (ESLint, tsc, prettier etc.).
  • My code has detailed inline documentation.
  • I have added unit tests to verify the code works as intended.
  • I have updated the project documentation accordingly.

@Swanand01
Copy link
Contributor Author

@ayushnirwal @justlevine What kind of validation should we add here:

blockDefinitions: {
type: 'object',
required: false,
},

@Swanand01
Copy link
Contributor Author

@justlevine Do we want to enforce lazy loading for the custom block implementations, or do we leave it up to the user to use React.lazy / normal imports?

On a side note, I haven't been able to verify if using React.lazy while passing block definitions is working as intended. Have discussed the same with @ayushnirwal over a call.

@Swanand01
Copy link
Contributor Author

This is blocked because we cannot import .tsx block definition files into snapwp.config.mjs.
The PR will be unblocked when we move snapwp.config to .ts

cc: @BhumikP

Copy link

changeset-bot bot commented Feb 20, 2025

⚠️ No Changeset found

Latest commit: 1138bbc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Swanand01 Swanand01 marked this pull request as ready for review February 20, 2025 08:52
Copy link
Contributor

@BhumikP BhumikP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the changes, LGTM

ayushnirwal
ayushnirwal previously approved these changes Feb 20, 2025
@ayushnirwal
Copy link
Collaborator

@Swanand01 I have updated the issue with the additional tasks discussed.

@Swanand01 Swanand01 requested a review from justlevine February 20, 2025 12:29
Comment on lines 18 to 20
export default function EditorBlocksRenderer( {
editorBlocks,
blockDefinitions,
}: EditorBlocksRendererProps ) {
Copy link
Collaborator

@justlevine justlevine Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pros vs cons about keeping this prop, and then having a hierarchy of props?.blockDefinitions || getConfig()?.blockDefinitions || defaultBlockDefinitions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you please tell me a case where the user would want to overwrite the block definitions passed from the config file by passing them as props to EditorBlocksRenderer?
I can't think of any pros of doing this at the moment.
Also, we are doing something similar to props?.blockDefinitions || defaultBlockDefinitions here:

public static attachRendererToTreeNode = ( node: BlockTreeNode ): void => {
const customBlockDefinition =
BlockManager.blockDefinitions[ node.type ];
const defaultBlockDefinition = defaultBlockDefinitions[ node.type ];
if ( customBlockDefinition === null ) {
// If explicitly set to null in custom definitions, use default renderer
node.renderer = BlockManager.blockDefinitions.default || Default;
} else if ( customBlockDefinition ) {
// If custom definition exists, use it
node.renderer = customBlockDefinition;
} else if ( defaultBlockDefinition ) {
// If no custom definition but default definition exists, use default
node.renderer = defaultBlockDefinition;
} else {
// If no definition found anywhere, use default renderer and prune children
node.renderer = BlockManager.blockDefinitions.default || Default;
node.children = null;
}
if ( node.children ) {
node.children.map( this.attachRendererToTreeNode );
}
};

Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

  • Should this PR include tests before merging? Why (not)?

It definitely requires an update our config-api.md, and overloading-wp-blocks.md, and a changeset (minor as is if we're removing the prop, patch if we're keeping it - changesets doesnt understand SemVer at <1.0.0)

@justlevine justlevine changed the title feat: migrate block definitions to getConfig feat: migrate block definitions to Config API Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants